-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid resetting Group transform by default #9525
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks
I agree with this PR
Please add a test covering this
And we merge
@asturur take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a spy to shouldResetTransform
and expect it it return false as well?
I was actually wondering if we should keep the |
I am open for that, |
How will we expose something like the following which belongs only to fixed layout ? setGroupDimensions(dims,origin): void
|
Maybe it should be a method on |
Also shouldLayoutClipPath... if we move layoutObjects to the strategy that can go away as well |
So i may agree with this pr or at least i don't disagree. |
I've refactored into using
That's for a separate PR.
Nothing secret. From the moment the Layout Manager has been introduced, we've migrated the groups to use it because it's much more easier to work with it reliably, without surprises with the Group auto-resizing. Then we carefully manually call A different perspective is realizing that with the Layout Manager, we have a first step towards a layout system, like the CSS one. So just like you can have an empty DIV with a fixed size and position, for whatever reason, we can wish for a Fixed Group, e.g. where to drop other objects as drop area. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So let's go for what you suggested in the first place and expose more of the lifecycle on the strategy
onBeforeLayout
, what else?
Added |
so let's focus one second on this transform reset. Then we have a fit content. |
i think this whole reset transform is an active selection issue and we can just have an active selection strategy, |
This sounds correct but needs to be tested, I am not sure what happens. Maybe I did that because of
ActiveSelection is a special entity with a set of expectations and the transform reset is part of it but the discussion is if it is part of how ActiveSelection behaves or how FitContent behaves, a discussion possible thanks to our work on extracting everything to a standalone. Our questionA group has transform and children. Expected BehaviorThe added object should seem unchanged after entering the group. ThoughtsI think that the group lost its transform when it lost the objects. |
updated my comment |
ok i think your question is good:
I would need to dig deep into 5.x to answer this question correctly, but my answer is that the group should have the transform it had before, because the group is in a developer driven flow so if the developer didn't touch scale, by default the developer should assume that no one touched scale. Before we refactored stuff the active selection was deleted as soon as it lost all objects and so the issue of resetting the transform wasn't there at all. But in general if you remove the objects from a group, you empty the objects array and that's it. Nothing else should happen to the group. ( fit content will change width/height, fixed layout not, clip-path layout not probably ). For this PR since now the situation is this one, and this PR is just changing the way we make this prebuilt flow and we exclude the fixed-layout from it, i don't see any issue. But my gut feeling is that it should be confined to the activeSelection. |
I can add more saying, that if you think the transform should be reset, just make a new group. if you want the transform to not be lost reuse the same group. |
const manager = new LayoutManager( | ||
reset ? new FitContentLayout() : new FixedLayout() | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this way the test isn't clear anymore.
please instead of true false in test.each, pass directly the strategy manager in the array, something like:
test.each([new FitContentLayout(), new FixedLayout()])('reset target transform %s', (strategyManager) => {
if it doesn't work with the title, just make 2 tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, but slightly differently. Please have a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart a change in tests i m good with this.
Please improve the test
urgh a circulare dependency |
This discussion is beneficial. Regarding FitContent and group transform... I agree that it is unexpected for group transform to suddenly reset so we should document this part as an expectation and how to handle it. |
Actually the only need I can think of for the active selection ref is event subscription. |
Regarding the blame. Then in #9152 I moved it to https://github.com/fabricjs/fabric.js/pull/9152/files#diff-dc26515afb0f87d2f6aa640d27b92a85ec523152d3aac8bc294ec59a66546295R262-R266 |
A few thoughts after reviewing this in depth.
I have local changes to push but tests are still looping infinately |
Have you pulled the latest changes? The only import that has changed is |
What we want to do with this PR? |
As we discussed I think the real issue here is my blame. |
Also we said we do not want fabric to change an object in unexpected ways. |
#9561 is merged => closing this and will remove |
Resetting the transform if the group is temporarily empty is very dangerous and unexpected, especially if
FixedLayout
is used. It makes sense only forFitContentLayout
IMO, where by definition if there is no content then the Group position is unknown.